-
Notifications
You must be signed in to change notification settings - Fork 81
fix #3461 #3474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #3461 #3474
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of questions in line, and one larger one: does your investigation suggest that the ssh/scp functionality is going to come back to github actions, or is it gone for good?
@@ -169,17 +169,20 @@ jobs: | |||
conda deactivate | |||
|
|||
echo "8. Setting up SSH" | |||
ssh-keygen -t rsa -b 4096 -N '' -f $PWD/qiita_ware/test/test_data/test_key | |||
ssh-keygen -t ed25519 -a 200 -N '' -f $PWD/qiita_ware/test/test_data/test_key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious: why was this switch necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading online they say that ed25519 vs rsa (just different encoding and in theory more secure) is necessary for github actions, which it wasn't in the past; thus decided to change it as part of the many trials to make it work. My thought was to leave it as this as it supposed to be the newer way
mkdir ~/.ssh/ | ||
cp $PWD/qiita_ware/test/test_data/test_key* ~/.ssh/ | ||
cat ~/.ssh/test_key.pub > ~/.ssh/authorized_keys | ||
cat ~/.ssh/test_key.pub > ~/.ssh/authorized_keys2 | ||
chmod 600 $PWD/qiita_ware/test/test_data/test_key* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does authorized_keys2
get used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should help: https://serverfault.com/a/116193. Similarly to the previous change, it's just to be sure that we have all the possible things to make this work.
scp -O -o StrictHostKeyChecking=no -i $PWD/qiita_ware/test/test_data/test_key $USER@localhost:/home/runner/work/qiita/qiita/qiita_ware/test/test_data/random_key /home/runner/work/qiita/qiita/qiita_ware/test/test_data/random_key_copy_1 | ||
|
||
# 05/22/25: commenting this line out as github actions is not allowing this step | ||
# scp -O -o StrictHostKeyChecking=no -i $PWD/qiita_ware/test/test_data/test_key $USER@localhost:/home/runner/work/qiita/qiita/qiita_ware/test/test_data/random_key /home/runner/work/qiita/qiita/qiita_ware/test/test_data/random_key_copy_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the effect of commenting this out? I see the comment above that says "this line is so the server is added to the list of known servers", so I assume that now it isn't added to the list of known servers :) ... but what effect does that have on behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the main culprit of the failings tests as it sets all the internal parameters for SCP to work by doing a single file scp; however, in all the builds it failed.
qiita_ware/test/test_commands.py
Outdated
@@ -56,26 +56,31 @@ def test_list_scp_wrong_key(self): | |||
list_remote('scp://runner@localhost:'+self.remote_dir_path, | |||
self.test_wrong_key) | |||
|
|||
def test_list_scp_nonexist_key(self): | |||
def test_download_remote_nonexist_key(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear to me why this switched from a test of list_remote
to a test of download_remote
... but if it is now for download_remote
, might be nice to move it down next to the other test for download_remote
(even though that one is currently commented out).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. This is testing a failure, both list_remote/download_remote have the same code base so they raise the same error. The only reason for the change was that after commenting out the tests, flake8 complained about download_remote not being used so decided to change this function so it's at least used here (just like list_remote in the other failure)
I don't know the answer for gone forever or not; my guess at this point is that github actions changed some internals on their firewall that is preventing this kind of tests - I was actually surprised they worked in the past. Sorry for not being more helpful on this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
No description provided.